feature: extend cri apis for get envs #2163
Conversation
starnop
force-pushed
the
cri-extend-env
branch
5 times, most recently
from
48a11a1 to
a3d4b03
Compare
starnop
changed the title
starnop
force-pushed
the
cri-extend-env
branch
3 times, most recently
from
96bd336 to
27df1ef
Compare
starnop
changed the title
Codecov Report
@@ Coverage Diff @@
## master #2163 +/- ##
=========================================
- Coverage 64.44% 64.4% -0.05%
=========================================
Files 209 209
Lines 16723 16735 +12
=========================================
+ Hits 10777 10778 +1
- Misses 4622 4630 +8
- Partials 1324 1327 +3
|
| @@ -774,6 +774,7 @@ func (c *CriManager) ContainerStatus(ctx context.Context, r *runtime.ContainerSt | |||
| Volumes: parseVolumesFromPouch(container.Config.Volumes), | |||
| Resources: parseResourcesFromPouch(resources, diskQuota), | |||
| QuotaId: container.Config.QuotaID, | |||
| Envs: parseEnvsFromPouch(container.Config.Env), | |||
There was a problem hiding this comment.
I do not think we should add Pouch in the function naming. Please change a proper one. @starnop
There was a problem hiding this comment.
@starnop Maybe we should rename the functions like parseEnvsFromPouch, parseResourcesFromPouch in another PR, they all do the same thing :)
@allencloud WDYT?
There was a problem hiding this comment.
👍 I will do that in the next pull request.
There was a problem hiding this comment.
I still insist that we could finish in the pull request. How about convertEnvsForCRI or convertContainerEnvForCRI?
cri/v1alpha2/cri_utils.go
Outdated
| for _, env := range pouchEnvs { | ||
| runtimeEnv := &runtime.KeyValue{} | ||
| if strings.Count(env, "=") == 1 { | ||
| envItem := strings.Split(env, "=") |
There was a problem hiding this comment.
What will happen if env is equal to a=b=c? Do we need to cover this case?
There was a problem hiding this comment.
@allencloud Actually the envs here is returned from pouchd and they should have been validated when input to pouchd.
So maybe we don't need to check again here.
There was a problem hiding this comment.
Yeah, in fact, the case a=b=c will not be passed by pouchd.
Even if that happens, the upper level can also deal with it by key.
There was a problem hiding this comment.
But you are using envItem[1] to take b and ignoring =c, right? @starnop
There was a problem hiding this comment.
a=b=c is valid because b=c is value.
# vagrant @ ubuntu-xenial in ~ [20:36:09]
$ A=1=2 echo ${A}
1=2
| @@ -1087,6 +1087,22 @@ func parseVolumesFromPouch(containerVolumes map[string]interface{}) map[string]* | |||
| return volumes | |||
| } | |||
|
|
|||
| // parseEnvsFromPouch parse Envs from []string to []*runtime.KeyValue | |||
| func parseEnvsFromPouch(pouchEnvs []string) (criEnvs []*runtime.KeyValue) { | |||
There was a problem hiding this comment.
you are not parsing, you just did the translation thing.
cri/v1alpha2/cri_utils.go
Outdated
| for _, env := range pouchEnvs { | ||
| runtimeEnv := &runtime.KeyValue{} | ||
| if strings.Count(env, "=") == 1 { | ||
| envItem := strings.Split(env, "=") |
There was a problem hiding this comment.
a=b=c is valid because b=c is value.
# vagrant @ ubuntu-xenial in ~ [20:36:09]
$ A=1=2 echo ${A}
1=2
27df1ef to
1d799d4
Compare
|
@fuweid Done. :) |
starnop
force-pushed
the
cri-extend-env
branch
4 times, most recently
from
21296b8 to
27e88f2
Compare
Signed-off-by: Starnop <starnop@163.com>
starnop
force-pushed
the
cri-extend-env
branch
from
27e88f2 to
59871c4
Compare
|
LGTM |
pouchrobot
added
the
LGTM
Signed-off-by: Starnop starnop@163.com
Ⅰ. Describe what this PR did
The CRI query container information interface adds the output of the environment variables, and returns the envs of the container in the
ContainerStatusinterfaceⅡ. Does this pull request fix one issue?
None.
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
Done.
Ⅳ. Describe how to verify it
crictl inspectto view the container's environment variables.Ⅴ. Special notes for reviews
None.